Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Review: CSS/JS Abstraction and Theme improvements #958

Merged

Conversation

rjmackay
Copy link
Contributor

@rjmackay rjmackay commented Dec 3, 2012

This a fairly major set of changes and could do with a serious code review.
Here's the main things this does:

  1. Add theme inheritance and css/js overriding
    • This still default to including the default theme
    • Allows themes to specify CSS/JS files to include through readme.txt
    • Allow themes to override CSS/JS from parent theme by include a file of the same name
  2. Split out themes/default/css/style.css
  3. Handle all CSS / JS includes through 1 library: Requirements
    • This enables us to combine and compress these files
    • Requirements library is ported from another framework, but has had serious modification so needs some review too (however I haven't rewritten it all so haven't enforced coding standards everywhere)
    • We're adding CSSMin and JSMin to compress files
    • A bunch of new options in application/config/requirements.php
  4. Add support for RTL css files through Requirements library.
    • All CSS files can be replace by a file of the same name with the -rtl suffix.

There are also other fixes that were enabled by the refactoring, and some general JS/CSS cleanup.

Issues addressed: #338 #550 #301 #261

I've started documenting all this here: https://wiki.ushahidi.com/display/WIKI/Managing+CSS+and+JS+in+Ushahidi

Any feedback appreciated, even if its to tell me I'm crazy and I've over thought the whole thing.

@@ -135,6 +135,8 @@ public function __construct()
$this->template->header->header_nav->loggedin_user = Auth::instance()->get_user();
}
$this->template->header->header_nav->site_name = Kohana::config('settings.site_name');

Event::add('ushahidi_filter.view_pre_render-layout', array($this, '_pre_render'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs for this hook?

As for the name, we've implicitly settled on using _ as a separator. So ushahidi_filter.view_pre_render_layout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a doc on this line.
This hook actually runs here: https://github.com/ushahidi/Ushahidi_Web/blob/develop/application/libraries/MY_View.php#L42
The last part -layout .. is based on the view name..
I've got - as a separator because I wanted something the split view_pre_render from the view name and have slightly less chance of naming collisions.

I can change that in MY_View.php if needed though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. In that case, perhaps a . separator? This way, the last part of a view_pre_render hook will always be the view name.



* pngFix can be run everywhere anyway.
* runScheduler was never being called
* Convert Themes library to use requirements
* Convert register_themes.php to use requirements
* Convert plugin helper to use requirements
* Move theme js to use Requirements library
* Stop passing script tag in api_url config settings.
  Set url in settings and handle the actual tag through Requirements
* Fix path for file to stop it suffix'ing remote urls
* Render Requirements through Themes library for frontend ushahidi#338
  This makes it easier to update old themes as they can still use header_block()
* Make admin/members layout call Requirement::render() since they
  don't have header_block()
* Only include jqplot js+css if needed
* Remove inline css and use class instead
* Remove graph div when disabled
* Rename style.css in other themes to avoid overriding default style.css ushahidi#301
* Move the Requirements calls from register_themes.php into Themes library
* Split style.css into base.css and style.css
* Tweak alerts radius map not to use .map class
  Use map-wrapper class instead as this avoids conflicts
  with div.map on main page. This Simplifies base.css so
  it doesn't need to style the map-wrapper too
* Move accordion styling to its own file
* Add basic style for reports submit and get alerts in base.css
* Remove some old unused CSS too
…shahidi#550

* Allow themes to specify addition css/js files to include in readme.txt
* Only include _default.css, style.css or THEMENAME.css by default
* Update existing themes to specify JS files
… css ushahidi#301

* Can now override openlayers.css and jquery-ui-themeroller.css
* themedCSS() will search for CSS in themes, then module (ie. plugin)
  then media/css
* Move iehacks CSS into default theme
* Added Requirements::ieCSS() and Requirements::ieThemedCSS()
* Also removing some unused images
* Remove duplicate images
* Remove duplicate jquery.timeago JS in ci_cumulus theme
…emes ushahidi#338

* Also updates all admin controller to set XYZ_enabled in themes
* Add requirements at the last minute through views_pre_render event
* Pass api_url_all as an array of urls, for easier use with Requirements
* Make Requirements handle arrays of css or js files
* Move some admin js to media/js/admin.js instead of inline
…hahidi#338

* Start calling requirements from pre-render event for main controller too
* Backend and frontend requirements are enabled through a variable on
  Themes.
* Slider, timeline and hovertip are now enabled like other js libraries
  rather than always included or include based on $main_page
* Removes unused flot js and excanvas js
* Openlayers and jquery-ui-themeroller are no longer themedcss but included
  early so can be overridden via theme css files anyway.
* Move admin header/footer block render to pre render event
* Replace jquery.ui.min.js with version including accordion
  Previously only included features used in the admin.
* No longer using the google CDN for this as per ushahidi#365
* Previously the admin used its own jquery UI, now both admin
  and frontend use the same version
Conflicts:

	application/views/admin/layout.php
* Pass themes object to themes_add_requirements action
* Add event for adding requirements before admin/frontend stuff
* Give all CSS/JS uniquenessIDs, based on filename if not supplied
* Require uniquenessID on custom CSS/JS/HeadTag
* Handle handle write_js_to_body option in Themes library not
  Requirements
* Give themedCSS a better uniqueid
* Split out Render by type instead of location
* Remove combined files code - rewrite later
* Remove auto js/css extension adding magic
* Add notice for missing JS/CSS files
* Add option to requirements to support combining files
* Add groups for base and admin combined files
* Move admin/all.css to admin.css so url() links work when combined
* Add CDN support for combined files
  * Auto upload combined files to the CDN
  * Fix minor issues in Cloudfiles classes -
    probably related to PHP 5.4
  * Make cdn helper handle files not located in media/uploads
    or already with full path.
* RTL languages are detected based on special core.text_direction
  i18n string
* If language is RTL we check for CSS files with the same name but a
  -rtl suffix
* Support -rtl versions of combined files too.
* Auto combine CSS files for themes
* Including the Minify_CSS_UriRewriter to fix url() links in theme css
* Include CSSMin to minify CSS files
* Add extra static call CSSMin::go()
Now using . as separator instead of -
As per feedback from @ekala this is a better match for the existing
naming convention.
* fix block() to handle IDs and full filenames
* fix path_for_file() to handle query strings w/o mtime
* fix delete_combined_files()
@rjmackay
Copy link
Contributor Author

Rebasing from develop and fixing conflicts.

kamaulynder added a commit that referenced this pull request Feb 15, 2013
…g-338

Code Review: CSS/JS Abstraction and Theme improvements
@kamaulynder kamaulynder merged commit 697d6ce into ushahidi:develop Feb 15, 2013
@kamaulynder
Copy link
Contributor

Applied the patch and after a few hitches with, finally they were cleanly applied. Looking good, functional tests will be done as we get close to tagging 2.7 for release.
@eyedol and I tested this and agreed we merge it in and handle anything after functional tests are done. Awesome work @rjmackay

@rjmackay
Copy link
Contributor Author

Ok great thanks for the effort!
on key this to test pre 2.7: what will this break in custom themes and can we avoid those breaks. I've had to shelve that for uchaguzi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants